Skip to content

feat(create-pr): add reuse_branch to keep one self-updating pull request - #80

Merged
marc0olo merged 10 commits into
mainfrom
feat/create-pr-reuse-branch
Sep 17, 2026
Merged

marc0olo merged 10 commits into
mainfrom
feat/create-pr-reuse-branch

Conversation

@marc0olo

@marc0olo marc0olo commented Aug 3, 2026 •

Copy link
Copy Markdown
Member

Closes #77.

create-pr adds a random suffix to branch_name on every run, so each run opens a new branch and a new pull request. While an earlier one is unmerged the next run still sees a diff and opens another, and after a release they all go permanently conflicting. icp-js-core accumulated 8 changelog pull requests for 2 distinct states; pic-js has 6 open, 3 conflicting.

Behaviour changes

  • New reuse_branch input on actions/create-pr. When enabled, branch_name is used as-is, the branch is reset and force pushed, the pull request already open for it is updated rather than duplicated, and it is closed once there is nothing left to propose. New pull_request_updated output.
  • generate-changelog enables it by default, so the five consuming repos move from one pull request per run to a single self-updating one.
  • That workflow now serialises its runs, keyed on the branch being pushed, so two runs cannot force push it concurrently.
  • create-pr refuses a branch_name equal to base_branch_name, which would otherwise have force pushed the base branch.
  • Git commands no longer go through a shell, and git push ends option parsing before the branch name. A branch name, commit message or author value can no longer be read as shell syntax, nor a branch name as a git option.

For the reviewer

--force rather than --force-with-lease: the branch is reset from the base every run, so a lease check would reject every push. Enabling reuse_branch asserts the branch is automation-owned, which the input description now says.

Not exercised live. The find, update and close paths need a real push to main in a consuming repo.

Two other callers have the same bug but keep false. npm-audit in icp-js-core is safe to flip. pull-project-docs in icp-js-sdk-docs is not: six projects share one branch name and each run resets to main after emptying only its own subdirectory, so reuse would discard another project's unmerged update. It needs per-project branch names first.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The updated git helpers still interpolate unquoted branch names into shell commands (execSync), which is a concrete command-injection risk when inputs are attacker-controlled.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a reuse_branch mode to the actions/create-pr action to keep a single self-updating PR (instead of opening a new PR on each run), and wires this behavior into the reusable generate-changelog workflow (including concurrency protection) to prevent changelog PR accumulation and long-lived conflicts.

Changes:

  • Add reuse_branch input plus “updated vs created” outputs, and implement PR find/update/close behavior in actions/create-pr.
  • Extend git helpers to support branch reset (git checkout -B) and force push (git push --force) when reusing a branch.
  • Enable reuse_branch by default for the generate-changelog reusable workflow and add a concurrency group to prevent branch force-push races; update READMEs accordingly.
File summaries
File Description
workflows/generate-changelog/README.md Document workflow behavior/inputs including reuse_branch and concurrency note.
.github/workflows/generate-changelog.yaml Add reuse_branch input defaulting to true and enforce concurrency to prevent races.
actions/create-pr/action.yaml Declare new reuse_branch input and new pull_request_updated output.
actions/create-pr/src/main.ts Implement branch reuse logic: stable head name, close obsolete PR on no-op, detect existing PR and mark as updated.
actions/create-pr/src/create-pull-request.ts Add helpers to find an open PR by head/base and close it when obsolete.
actions/create-pr/src/create-commit.ts Reset and force-push when reusing a long-lived branch.
actions/create-pr/README.md Document new input/output semantics.
lib/action-utils/src/git.ts Add options to checkout/push helpers to support reset and force push.
actions/create-pr/dist/index.js Rebuilt bundle reflecting new create-pr behavior.
actions/assemble-docs/dist/index.js Rebuilt bundle reflecting updated git helpers.
actions/submit-docs/dist/index.js Rebuilt bundle reflecting updated git helpers.
actions/extract-version/dist/index.js Rebuilt bundle reflecting updated git helpers.
Review details
  • Files reviewed: 8/12 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/action-utils/src/git.ts
Comment thread lib/action-utils/src/git.ts
Comment thread actions/create-pr/README.md Outdated
Comment thread workflows/generate-changelog/README.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

reuse_branch introduces a force-push path without guarding against branch_name === base_branch_name, which could reset and force-push the base branch if misconfigured.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 9/13 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread actions/create-pr/src/main.ts
@marc0olo

Copy link
Copy Markdown
Member Author

Valid and fixed in 78061bb.

With the random suffix, head could never equal base, so the force-push path was unreachable. Reuse removes the suffix and uses branch_name verbatim, so branch_name: main would have run git checkout -B main followed by git push --force -u origin main — force-pushing the base branch. Both inputs are caller-settable on the reusable workflow, so it is a realistic misconfiguration with a very bad outcome.

create-pr now refuses it before any git operation runs:

if (head === base) {
  throw new Error(
    `branch_name resolves to the base branch '${base}'. Set branch_name to a different branch.`,
  );
}

The guard sits at main.ts:36, ahead of gitHasChanges (45) and createCommit (77), so nothing touches the repository first. It is unconditional rather than gated on reuse_branch, since a head equal to base is invalid either way — GitHub returns 422 for such a pull request — and an unconditional check has no way to be bypassed by a future caller.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

gitPushBranch still allows git option injection via branch names beginning with -- unless an end-of-options marker is added before the refspec.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

lib/action-utils/src/git.ts:44

  • gitPushBranch passes the branch name as the final argument to git push without an explicit -- end-of-options marker. If the input branch name begins with -/-- (e.g. --all, --mirror), Git can parse it as an option rather than a refspec, potentially pushing unintended refs or otherwise changing push behavior. Add -- before the branch/refspec to prevent option injection and to support such branch names safely.
  • Files reviewed: 9/13 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@marc0olo

Copy link
Copy Markdown
Member Author

Valid, fixed in abdd46c. Confirmed the behaviour rather than assuming it:

$ git push --force -u origin --mirror
To ./remote.git
 * [new branch]      main -> main          <- parsed as the --mirror OPTION

$ git push --force -u origin -- --mirror
error: src refspec --mirror does not match any    <- now a refspec

So -- is the right remedy and gitPushBranch now passes it.

Two things testing changed about the fix. It is limited to the push, because -- breaks checkout: -B takes its value positionally, so git checkout -B -- <name> fails with option `orphan' requires a value. And it needs no marker anyway — git validates the name itself:

$ git checkout -B --mirror
fatal: '--mirror' is not a valid branch name

git config user.name --global was worth checking too, and assigns the literal string rather than parsing an option, so it is not a vector.

On severity, to be accurate rather than alarming: this was not reachable through create-pr. createCommit runs gitCheckoutBranch before gitPushBranch, and that checkout rejects a dash-leading name and throws, so the push never ran. The fix is worth making because gitPushBranch is exported from @dfinity/action-utils and another action could call it without a preceding checkout — a shared utility should not depend on its caller's ordering for that.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

gitCheckoutBranch still lacks a -- end-of-options separator, allowing branch names beginning with - to be parsed as git options (option-injection risk) despite the other hardening work.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

lib/action-utils/src/git.ts:46

  • This comment is inaccurate: git checkout -b/-B does have an equivalent -- end-of-options separator (and should use it) to prevent branch names starting with - from being parsed as options. Leaving this as-is is misleading for future maintenance.
  // `--` keeps a branch name that begins with a dash from being read as an
  // option: `git push origin --mirror` pushes every ref rather than a branch.
  // `git checkout -B` needs no equivalent, and rejects such a name itself.
  • Files reviewed: 9/13 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread lib/action-utils/src/git.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new concurrency grouping does not fully prevent force-push races when the reusable workflow is invoked from multiple refs using the same reused branch_name.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/generate-changelog.yaml:87

  • The concurrency group is keyed on github.ref (the triggering ref), but the race you’re preventing is on the reused head branch (inputs.branch_name). If this reusable workflow is invoked for multiple refs (e.g. main + a release branch) while keeping the same branch_name, runs can still overlap and force-push the same branch concurrently.
    workflows/generate-changelog/README.md:11
  • This note explains that runs are serialized to avoid races when reuse_branch reuses a single branch, but it doesn’t mention that the reusable workflow itself now sets concurrency. Callers reading this README may still add their own concurrency block unnecessarily (and potentially with a different grouping).
  • Files reviewed: 9/13 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@marc0olo

Copy link
Copy Markdown
Member Author

Both suppressed comments were worth it, and the first one is a real bug in my own change. Fixed in 6f12813.

The concurrency key was wrong. The group was ${{ github.workflow }}-${{ github.ref }}, but the contended resource is the reused head branch, not the ref that triggered the run. Two refs invoking this workflow with the same branch_name land in separate groups and force push the same branch concurrently — exactly the race the group was added to prevent.

-  group: ${{ github.workflow }}-${{ github.ref }}
+  group: generate-changelog-${{ inputs.branch_name }}

base_branch_name is deliberately not in the key. Including it would split runs that share one branch_name across different bases, which is the same mistake in a different form — they still push the same ref. Dropped github.workflow too, so the key identifies the branch regardless of what a caller names its own workflow.

Checked that inputs is an allowed context in concurrency before relying on it, and that workflow level in the called workflow is the right place to set it rather than on the calling job.

The README point was fair as well. It described the effect without saying the workflow sets concurrency itself, so a caller could reasonably add their own with a different grouping. It now says so explicitly, and names the key.

Worth recording that this is the fourth pass on this PR and the first three each found something real: shell injection, the base-branch force push, then option injection on push. This one found a defect in the fix for the third. The one I declined was the only miss.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The updated documentation for the reusable generate-changelog workflow and create-pr action contains incorrect defaults/usage that can cause copy-paste or “rely on defaults” misconfiguration.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

actions/create-pr/README.md:27

  • The README lists the default token as ${{ GITHUB_TOKEN }}, but that expression is not valid in workflow syntax (it should be ${{ github.token }} or ${{ secrets.GITHUB_TOKEN }}). Aligning this with action.yaml prevents copy/paste failures.

workflows/generate-changelog/README.md:21

  • The documented defaults for pull_request_title / pull_request_body don’t match the workflow’s actual defaults in .github/workflows/generate-changelog.yaml (which are changelog-specific). Keeping these aligned avoids surprising PR titles/bodies when callers rely on defaults.
| `pull_request_title` | The title of the pull request.                                                                | `'chore: automated by GitHub actions'`                                |
| `pull_request_body`  | The body of the pull request.                                                                 | `'This pull request was automatically created by a GitHub Action.'`   |

workflows/generate-changelog/README.md:24

  • The documented default for commit_message does not match the workflow’s actual default in .github/workflows/generate-changelog.yaml (it defaults to chore: generate changelog).
| `commit_message`     | The message of the commit.                                                                    | `'chore: automated by GitHub actions'`                                |
  • Files reviewed: 9/13 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread workflows/generate-changelog/README.md Outdated
marc0olo and others added 7 commits September 17, 2026 15:05
Reusing a branch resets and force pushes it, so a branch_name equal to
base_branch_name would have force pushed the base branch. The random
suffix made that unreachable before, and reuse removes it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Passing arguments directly stops a shell reading them, but git still
parses a leading dash as an option: `git push origin --mirror` pushes
every ref rather than a branch of that name.

git checkout -B needs no marker and rejects such a name itself, so the
change is limited to the push.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch is a trailing positional on push and the value of an option
on checkout, which is why only one of them needs the marker. Stating the
mechanism rather than the conclusion, since the previous wording read as
an unexplained asymmetry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The group used the triggering ref, but the contended resource is the
reused head branch. Two refs invoking the workflow with the same
branch_name landed in separate groups and could still force push it
concurrently, which is the race the group was added to prevent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reusable workflow's input table was copied from the create-pr action
and never updated, so branch_name, the pull request title and body, and
the commit message all documented the action's defaults rather than the
workflow's. A caller relying on them would have got a different branch
and different commit text than described.

The action's token default was also written as ${{ GITHUB_TOKEN }},
which is not valid workflow syntax.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The input described updating a pull request without mentioning that the
branch is reset and force pushed first, which is the operationally
significant part and the reason it suits only a branch owned by
automation. Stated in the input description and in both READMEs, the
reusable workflow included since it enables this by default.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
environment is required but was absent from both the input table and
the example, so the example failed when copied. file_name, owner and
repository were missing as well.

The example also carried a caller concurrency block, which the workflow
now provides itself keyed on the branch it pushes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@marc0olo
marc0olo merged commit 33c6fa2 into main Sep 17, 2026
14 checks passed
@marc0olo
marc0olo deleted the feat/create-pr-reuse-branch branch September 17, 2026 13:18
marc0olo added a commit that referenced this pull request Sep 17, 2026
#80 changed actions/create-pr, so the workflows still referenced the
copy from before it and reuse_branch would have been passed to an action
that does not declare it.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sea-snake pushed a commit to dfinity/icp-js-auth that referenced this pull request Sep 25, 2026
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit.

Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request
is force pushed on every push to `main`, and those pushes came from the
default token as `github-actions[bot]`. Its workflows then waited for
approval, and required `pull_request_target` workflows such as the
External PR Ruleset never ran, which blocks the pull request even when
approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with
the app token instead.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
marc0olo added a commit to dfinity/icp-js-bindgen that referenced this pull request Sep 25, 2026
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit.

Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request
is force pushed on every push to `main`, and those pushes came from the
default token as `github-actions[bot]`. Its workflows then waited for
approval, and required `pull_request_target` workflows such as the
External PR Ruleset never ran, which blocks the pull request even when
approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with
the app token instead.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
marc0olo added a commit to dfinity/icp-js-sdk-docs that referenced this pull request Sep 25, 2026
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit.

Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request
is force pushed on every push to `main`, and those pushes came from the
default token as `github-actions[bot]`. Its workflows then waited for
approval, and required `pull_request_target` workflows such as the
External PR Ruleset never ran, which blocks the pull request even when
approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with
the app token instead.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
sea-snake pushed a commit to dfinity/icp-js-signer that referenced this pull request Sep 25, 2026
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit.

Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request
is force pushed on every push to `main`, and those pushes came from the
default token as `github-actions[bot]`. Its workflows then waited for
approval, and required `pull_request_target` workflows such as the
External PR Ruleset never ran, which blocks the pull request even when
approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with
the app token instead.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
marc0olo added a commit to dfinity/icp-js-core that referenced this pull request Sep 25, 2026
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit.

Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request
is force pushed on every push to `main`, and those pushes came from the
default token as `github-actions[bot]`. Its workflows then waited for
approval, and required `pull_request_target` workflows such as the
External PR Ruleset never ran, which blocks the pull request even when
approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with
the app token instead.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

generate-changelog: new branch per run causes changelog PRs to accumulate and go permanently conflicting

3 participants